Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang] Implement CWG2398 provisional TTP matching to class templates #92855

Merged

Conversation

mizvekov
Copy link
Contributor

This solves some ambuguity introduced in P0522 regarding how template template parameters are partially ordered, and should reduce the negative impact of enabling -frelaxed-template-template-args by default.

When performing template argument deduction, we extend the provisional wording introduced in #89807 so it also covers deduction of class templates.

Given the following example:

template <class T1, class T2 = float> struct A;
template <class T3> struct B;

template <template <class T4> class TT1, class T5> struct B<TT1<T5>>;   // #1
template <class T6, class T7>                      struct B<A<T6, T7>>; // #2

template struct B<A<int>>;

Prior to P0522, #2 was picked. Afterwards, this became ambiguous. This patch restores the pre-P0522 behavior, #2 is picked again.

This has the beneficial side effect of making the following code valid:

template<class T, class U> struct A {};
A<int, float> v;
template<template<class> class TT> void f(TT<int>);

// OK: TT picks 'float' as the default argument for the second parameter.
void g() { f(v); }

Since this changes provisional implementation of CWG2398 which has not been released yet, and already contains a changelog entry, we don't provide a changelog entry here.

@mizvekov mizvekov self-assigned this May 21, 2024
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels May 21, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented May 21, 2024

@llvm/pr-subscribers-clang

Author: Matheus Izvekov (mizvekov)

Changes

This solves some ambuguity introduced in P0522 regarding how template template parameters are partially ordered, and should reduce the negative impact of enabling -frelaxed-template-template-args by default.

When performing template argument deduction, we extend the provisional wording introduced in #89807 so it also covers deduction of class templates.

Given the following example:

template &lt;class T1, class T2 = float&gt; struct A;
template &lt;class T3&gt; struct B;

template &lt;template &lt;class T4&gt; class TT1, class T5&gt; struct B&lt;TT1&lt;T5&gt;&gt;;   // #<!-- -->1
template &lt;class T6, class T7&gt;                      struct B&lt;A&lt;T6, T7&gt;&gt;; // #<!-- -->2

template struct B&lt;A&lt;int&gt;&gt;;

Prior to P0522, #<!-- -->2 was picked. Afterwards, this became ambiguous. This patch restores the pre-P0522 behavior, #<!-- -->2 is picked again.

This has the beneficial side effect of making the following code valid:

template&lt;class T, class U&gt; struct A {};
A&lt;int, float&gt; v;
template&lt;template&lt;class&gt; class TT&gt; void f(TT&lt;int&gt;);

// OK: TT picks 'float' as the default argument for the second parameter.
void g() { f(v); }

Since this changes provisional implementation of CWG2398 which has not been released yet, and already contains a changelog entry, we don't provide a changelog entry here.


Full diff: https://github.com/llvm/llvm-project/pull/92855.diff

4 Files Affected:

  • (modified) clang/lib/Sema/SemaTemplate.cpp (+4-1)
  • (modified) clang/lib/Sema/SemaTemplateDeduction.cpp (+44-28)
  • (modified) clang/test/CXX/temp/temp.decls/temp.alias/p2.cpp (+3-2)
  • (modified) clang/test/SemaTemplate/cwg2398.cpp (-3)
diff --git a/clang/lib/Sema/SemaTemplate.cpp b/clang/lib/Sema/SemaTemplate.cpp
index 6af35ac8911bb..b7479cbcdba23 100644
--- a/clang/lib/Sema/SemaTemplate.cpp
+++ b/clang/lib/Sema/SemaTemplate.cpp
@@ -1807,6 +1807,8 @@ static void SetNestedNameSpecifier(Sema &S, TagDecl *T,
 // Returns the template parameter list with all default template argument
 // information.
 static TemplateParameterList *GetTemplateParameterList(TemplateDecl *TD) {
+  if (TD->isImplicit())
+    return TD->getTemplateParameters();
   // Make sure we get the template parameter list from the most
   // recent declaration, since that is the only one that is guaranteed to
   // have all the default template argument information.
@@ -1827,7 +1829,8 @@ static TemplateParameterList *GetTemplateParameterList(TemplateDecl *TD) {
   //    template <class = void> friend struct C;
   //  };
   //  template struct S<int>;
-  while (D->getFriendObjectKind() != Decl::FriendObjectKind::FOK_None &&
+  while ((D->isImplicit() ||
+          D->getFriendObjectKind() != Decl::FriendObjectKind::FOK_None) &&
          D->getPreviousDecl())
     D = D->getPreviousDecl();
   return cast<TemplateDecl>(D)->getTemplateParameters();
diff --git a/clang/lib/Sema/SemaTemplateDeduction.cpp b/clang/lib/Sema/SemaTemplateDeduction.cpp
index f16a07e1a1b34..791e44658bd96 100644
--- a/clang/lib/Sema/SemaTemplateDeduction.cpp
+++ b/clang/lib/Sema/SemaTemplateDeduction.cpp
@@ -583,37 +583,53 @@ DeduceTemplateArguments(Sema &S, TemplateParameterList *TemplateParams,
       return TemplateDeductionResult::Success;
 
     auto NewDeduced = DeducedTemplateArgument(Arg);
-    // Provisional resolution for CWG2398: If Arg is also a template template
-    // param, and it names a template specialization, then we deduce a
-    // synthesized template template parameter based on A, but using the TS's
-    // arguments as defaults.
-    if (auto *TempArg = dyn_cast_or_null<TemplateTemplateParmDecl>(
-            Arg.getAsTemplateDecl())) {
+    // Provisional resolution for CWG2398: If Arg names a template
+    // specialization, then we deduce a synthesized template template parameter
+    // based on A, but using the TS's arguments as defaults.
+    if (DefaultArguments.size() != 0) {
       assert(Arg.getKind() == TemplateName::Template);
-      assert(!TempArg->isExpandedParameterPack());
-
+      TemplateDecl *TempArg = Arg.getAsTemplateDecl();
       TemplateParameterList *As = TempArg->getTemplateParameters();
-      if (DefaultArguments.size() != 0) {
-        assert(DefaultArguments.size() <= As->size());
-        SmallVector<NamedDecl *, 4> Params(As->size());
-        for (unsigned I = 0; I < DefaultArguments.size(); ++I)
-          Params[I] = getTemplateParameterWithDefault(S, As->getParam(I),
-                                                      DefaultArguments[I]);
-        for (unsigned I = DefaultArguments.size(); I < As->size(); ++I)
-          Params[I] = As->getParam(I);
-        // FIXME: We could unique these, and also the parameters, but we don't
-        // expect programs to contain a large enough amount of these deductions
-        // for that to be worthwhile.
-        auto *TPL = TemplateParameterList::Create(
-            S.Context, SourceLocation(), SourceLocation(), Params,
-            SourceLocation(), As->getRequiresClause());
-        NewDeduced = DeducedTemplateArgument(
-            TemplateName(TemplateTemplateParmDecl::Create(
-                S.Context, TempArg->getDeclContext(), SourceLocation(),
-                TempArg->getDepth(), TempArg->getPosition(),
-                TempArg->isParameterPack(), TempArg->getIdentifier(),
-                TempArg->wasDeclaredWithTypename(), TPL)));
+      assert(DefaultArguments.size() <= As->size());
+
+      SmallVector<NamedDecl *, 4> Params(As->size());
+      for (unsigned I = 0; I < DefaultArguments.size(); ++I)
+        Params[I] = getTemplateParameterWithDefault(S, As->getParam(I),
+                                                    DefaultArguments[I]);
+      for (unsigned I = DefaultArguments.size(); I < As->size(); ++I)
+        Params[I] = As->getParam(I);
+      // FIXME: We could unique these, and also the parameters, but we don't
+      // expect programs to contain a large enough amount of these deductions
+      // for that to be worthwhile.
+      auto *TPL = TemplateParameterList::Create(
+          S.Context, SourceLocation(), SourceLocation(), Params,
+          SourceLocation(), As->getRequiresClause());
+
+      TemplateDecl *TD;
+      switch (TempArg->getKind()) {
+      case Decl::TemplateTemplateParm: {
+        auto *A = cast<TemplateTemplateParmDecl>(TempArg);
+        assert(!A->isExpandedParameterPack());
+        TD = TemplateTemplateParmDecl::Create(
+            S.Context, A->getDeclContext(), SourceLocation(), A->getDepth(),
+            A->getPosition(), A->isParameterPack(), A->getIdentifier(),
+            A->wasDeclaredWithTypename(), TPL);
+        break;
+      }
+      case Decl::ClassTemplate: {
+        auto *A = cast<ClassTemplateDecl>(TempArg);
+        auto *CT = ClassTemplateDecl::Create(S.Context, A->getDeclContext(),
+                                             SourceLocation(), A->getDeclName(),
+                                             TPL, A->getTemplatedDecl());
+        CT->setPreviousDecl(A);
+        TD = CT;
+        break;
+      }
+      default:
+        llvm_unreachable("Unexpected Template Kind");
       }
+      TD->setImplicit(true);
+      NewDeduced = DeducedTemplateArgument(TemplateName(TD));
     }
 
     DeducedTemplateArgument Result = checkDeducedTemplateArguments(S.Context,
diff --git a/clang/test/CXX/temp/temp.decls/temp.alias/p2.cpp b/clang/test/CXX/temp/temp.decls/temp.alias/p2.cpp
index a5b39fe5c51f7..bc39431253880 100644
--- a/clang/test/CXX/temp/temp.decls/temp.alias/p2.cpp
+++ b/clang/test/CXX/temp/temp.decls/temp.alias/p2.cpp
@@ -28,13 +28,14 @@ namespace StdExample {
     { /* ... */ }
 
   template<template<class> class TT>
-    void f(TT<int>); // expected-note {{candidate template ignored}}
+    void f(TT<int>);
 
   template<template<class,class> class TT>
     void g(TT<int, Alloc<int>>);
 
   int h() {
-    f(v); // expected-error {{no matching function for call to 'f'}}
+    f(v); // OK: TT = vector, Alloc<int> is used as the default argument for the
+          // second parameter.
     g(v); // OK: TT = vector
   }
 
diff --git a/clang/test/SemaTemplate/cwg2398.cpp b/clang/test/SemaTemplate/cwg2398.cpp
index e3b5e575374d3..4cc946735a1e2 100644
--- a/clang/test/SemaTemplate/cwg2398.cpp
+++ b/clang/test/SemaTemplate/cwg2398.cpp
@@ -65,13 +65,10 @@ namespace class_template {
   template <class T3> struct B;
 
   template <template <class T4> class TT1, class T5> struct B<TT1<T5>>;
-  // new-note@-1 {{partial specialization matches}}
 
   template <class T6, class T7> struct B<A<T6, T7>> {};
-  // new-note@-1 {{partial specialization matches}}
 
   template struct B<A<int>>;
-  // new-error@-1 {{ambiguous partial specialization}}
 } // namespace class_template
 
 namespace type_pack1 {

Copy link
Collaborator

@erichkeane erichkeane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable, lgtm.

@mizvekov mizvekov force-pushed the users/mizvekov/clang-type-param-default-argument branch from 29f6855 to 142c3f3 Compare May 21, 2024 22:56
Base automatically changed from users/mizvekov/clang-type-param-default-argument to main May 21, 2024 23:27
This solves some ambuguity introduced in P0522 regarding how
template template parameters are partially ordered, and should reduce
the negative impact of enabling `-frelaxed-template-template-args`
by default.

When performing template argument deduction, we extend the provisional
wording introduced in #89807
so it also covers deduction of class templates.

Given the following example:
```C++
template <class T1, class T2 = float> struct A;
template <class T3> struct B;

template <template <class T4> class TT1, class T5> struct B<TT1<T5>>;   // #1
template <class T6, class T7>                      struct B<A<T6, T7>>; // #2

template struct B<A<int>>;
```
Prior to P0522, `#2` was picked. Afterwards, this became ambiguous.
This patch restores the pre-P0522 behavior, `#2` is picked again.

This has the beneficial side effect of making the following code valid:
```C++
template<class T, class U> struct A {};
A<int, float> v;
template<template<class> class TT> void f(TT<int>);

// OK: TT picks 'float' as the default argument for the second parameter.
void g() { f(v); }
```

---

Since this changes provisional implementation of CWG2398 which has
not been released yet, and already contains a changelog entry,
we don't provide a changelog entry here.
@mizvekov mizvekov changed the base branch from main to users/mizvekov/clang-nttp-default-argument May 22, 2024 00:51
@mizvekov mizvekov force-pushed the users/mizvekov/clang-cwg2398-ttp-matches-class-template branch from de5b6aa to 73d456c Compare May 22, 2024 00:51
Base automatically changed from users/mizvekov/clang-nttp-default-argument to main May 22, 2024 15:18
mizvekov added a commit that referenced this pull request May 22, 2024
This is an enabler for #92855

This allows an NTTP default argument to be set as an arbitrary
TemplateArgument, not just an expression.
This allows template parameter packs to have default arguments in the
AST, even though the language proper doesn't support the syntax for it.

This allows NTTP default arguments to be other kinds of arguments, like
packs, integral constants, and such.
@mizvekov mizvekov merged commit ff3f41d into main May 22, 2024
3 checks passed
@mizvekov mizvekov deleted the users/mizvekov/clang-cwg2398-ttp-matches-class-template branch May 22, 2024 18:59
@jyknight
Copy link
Member

Heads up: this commit has triggered some weird errors for a compile, but only when clang header modules are enabled. A std::vector, that's built through a large amount of template gunk that definitely involves template template parameter matching, ends up failing vector's "Allocator::value_type must be same type as value_type" static assertion -- the vector's allocator's value_type seems to be some unrelated type. But, only when modules are enabled.

That seems to probably indicate a bug in this commit (presumably related to AST serialization or deserialization?), but I don't have a test-case, and, that's harder to get for issues that only arise with modules enabled...

If this very vague and unhelpful description makes you think "oh, I see where the problem likely is!", then great! But, otherwise, I'll see if I can create a test case...

@dcci
Copy link
Member

dcci commented May 23, 2024

I am seeing something similar in vector (without modules). I'm reducing a test case now.

@dcci
Copy link
Member

dcci commented May 23, 2024

Example error while I reduce:

fbcode/third-party-buck/platform010/build/libgcc/include/c++/trunk/bits/stl_vector.h:404:21: error: static assertion failed due to requirement 'is_same<std::basic_string<char, std::char_traits<char>, std::allocator<char>>, quic::Interval<unsigned long, 1>>::value': std::vector must have the same value_type as its allocator
  404 |       static_assert(is_same<typename _Alloc::value_type, _Tp>::value,
      |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
buck-out/v2/gen/fbcode/21eca57c08ba71f9/quic/common/__interval_set__/buck-headers/quic/common/IntervalSet.h:61:29: note: in instantiation of template class 'std::vector<quic::Interval<unsigned long>>' requested here
   61 | class IntervalSet : private Container<Interval<T, Unit>> {
      |                             ^
buck-out/v2/gen/fbcode/21eca57c08ba71f9/quic/state/__quic_state_machine__/buck-headers/quic/state/QuicStreamManager.h:79:41: note: in instantiation of template class 'quic::IntervalSet<unsigned long, 1, std::vector>' requested here
   79 |   IntervalSet<StreamId, 1, std::vector> streams_;
      |                                         ^

@mizvekov
Copy link
Contributor Author

Thanks for the heads up, this does look like a problem. I am reverting it for now.

@mizvekov mizvekov restored the users/mizvekov/clang-cwg2398-ttp-matches-class-template branch May 24, 2024 00:30
mizvekov added a commit that referenced this pull request May 24, 2024
…emplates" (#93258)

Reverts #92855

This is causing issues, there are still being reduced, but does look
like a problem.

See PR for user reports.
@mizvekov
Copy link
Contributor Author

Heads up: this commit has triggered some weird errors for a compile, but only when clang header modules are enabled.
That seems to probably indicate a bug in this commit (presumably related to AST serialization or deserialization?), but I don't have a test-case, and, that's harder to get for issues that only arise with modules enabled...

Thanks for the heads-up, James.

Indeed, and it's not only modules, our tooling for reducing test cases is getting less and less effective.

@mizvekov
Copy link
Contributor Author

I figured a reproducer based on your hints:

template<class T, class U> struct A {};

template<template<class> class TT> auto f(TT<int> a) { return a; }

A<int, float> v1;
A<int, double> v2;

using X = decltype(f(v1));
using Y = decltype(f(v2));

Fails with:

t.cc:9:20: error: no matching function for call to 'f'
    9 | using Y = decltype(f(v2));
      |                    ^
t.cc:3:41: note: candidate template ignored: deduced type 'A<[...], (default) float>' of 1st parameter does not match adjusted type 'A<[...], double>' of argument [with TT = A]
    3 | template<template<class> class TT> auto f(TT<int> a) { return a; }
      |                                         ^

This shows we are not properly canonicalizing the synthesized template declaration.

The problem does not show up with class template partial specializations due to a pre-existing issue, but otherwise this was an oversight on my part.

No need to keep reducing on your end, I already have enough to go with.

searlmc1 pushed a commit to ROCm/llvm-project that referenced this pull request May 25, 2024
mirrors in upstream revert of :
ff3f41d [clang] Implement CWG2398 provisional TTP matching to class templates (llvm#92855)

Change-Id: I233989add55dd8605271a2577a8595153725c17a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants